Pull down the replace method from ShadowList to Candidates #553
Conversation
|
All test are green so from my side it would be ready to merge! |
bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/Candidates.java
Outdated
Show resolved
Hide resolved
laeubi
force-pushed
the
pull_down_replace
branch
from
acd09a4 to
711db22
Compare
|
@tjwatson thanks for the explanation and hints, I have now adjusted the PR to reflect the original idea of the ShadowList method. |
|
Okay looking at the javadoc it even says
This is already violated as we make a copy of the list (even copied twice) so it can never be the list, I also made the list unmodifiable here and it shows that Equinox at least even inserts items in the list directly, what seems valid but not mandatory, so if a resolve context really uses a cached list and only inserts items in this cache, or even relies on identity lookup this will break. This also seems not really covered by the TCK, so I think before going further here we first need a TCK, then make sure Felix passes the TCK and then go on. Taking this into account it seems the resolver would need to build the datastructure in a two step way, what likely will also make it easier to "finalize it":
There is just one tricky part |
laeubi
force-pushed
the
pull_down_replace
branch
from
711db22 to
22fe7be
Compare
bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/util/ShadowedCapability.java
Outdated
Show resolved
Hide resolved
bundles/org.eclipse.osgi/felix/src/org/apache/felix/resolver/util/ShadowedCapability.java
Show resolved
Hide resolved
| copy[i] = ((ShadowedCapability) capability).getShadowed(); | ||
| } | ||
| } | ||
| int insertIdx = context.insertHostedCapability(Arrays.asList(copy), toInsertCapability); |
There was a problem hiding this comment.
This seems odd and not in the spirit of the specification when multiple hosted capabilities are inserted into the list. The ResolveContext will keep getting the same original list back and it will not have the past hosted capabilities there that the ResolveContext had inserted. This means the ResolveContext will not have any control over the order of the inserted HostedCapability objects in relation to each other.
There was a problem hiding this comment.
I now fixed that, the main problem is that Felix resolver is violation the contract of insertHosteCapability, I added a TCK that shows the problem, but at least this should work as before.
The replace method is currently used on one place and the reason we construct a new ShadowList, to remove references to this class the method can simply pulls down to Candidates as it only operates on the internal unmodifiable list. This also adds a check that if the Capability to replace does not exits it is a noop to call this method instead of running into IndexOutOfBoundsException.
laeubi
force-pushed
the
pull_down_replace
branch
from
22fe7be to
bab16fe
Compare
|
I'll close this for now and maybe came up with a different aproach. |
The replace method is currently used on one place and the reason we construct a new ShadowList, to remove references to this class the method can simply pulls down to Candidates as it only operates on the internal unmodifiable list.
This also adds a check that if the Capability to replace does not exits it is a noop to call this method instead of running into IndexOutOfBoundsException.
@tjwatson one small step to get rid of direct references to
ShadowListas a preliminary step for further refactorings.